fix: Convert full source paths to relative in logs #8823

Aiiaiiio · 2025-10-02T09:43:42Z

This is a quick fix to convert the full source paths to relative ones in log files. Saves a bit of spaces and prevents a security risk (leaking CI paths into logs).

I understand that the solution is a bit wonky. I've looked at other possibilities and found this the least bad. Suggestions welcome.

Signed-off-by: Tamás Bari <[email protected]>

mgallien · 2025-10-02T10:08:43Z

CMakeLists.txt


 get_git_head_revision(GIT_REFSPEC GIT_SHA1)

+add_definitions(-DSOURCE_ROOT="${CMAKE_SOURCE_DIR}")


please no
this should be properly defined in config.h.in
if you work on the code, and have this global define you cannot figure out where it is coming from
in addition, we should always use the cmake so-called target APIs
https://cmake.org/cmake/help/latest/command/target_compile_definitions.html for example

mgallien · 2025-10-02T10:11:57Z

src/libsync/logger.cpp

+    QString msg = qFormatLogMessage(type, ctx, message);
+
+    // We want to change the full path of the source file to relative,
+    // to reduce log size and also, to not leak paths into logs.
+    // To help with this, there is a placeholder in the message pattern
+    // for the file path ("<file-path-here>").
+    QString filePath;
+    if (ctx.file != nullptr) {
+        static const QString projectRoot = QStringLiteral(SOURCE_ROOT);
+
+        filePath = QFileInfo(QString::fromLocal8Bit(ctx.file)).absoluteFilePath();
+        if (filePath.startsWith(projectRoot)) {
+            filePath = filePath.mid(projectRoot.size() + 1);
+        }
+    }
+
+    static const QString filePathPlaceholder = "<file-path-here>";
+    msg.replace(filePathPlaceholder, filePath);
+


I am really concerned about the performance implication of that change
we already are a bit slow when printing logs
maybe you could use a hash map to have a quick mapping from absolute paths to relative ones and just add this info somewhere in the log line

Instead of trying to convert them to relative paths. Signed-off-by: Tamás Bari <[email protected]>

Signed-off-by: Tamás Bari <[email protected]>

sonarqubecloud · 2025-10-02T14:12:43Z

Quality Gate failed

Failed conditions
11 New Code Smells (required ≤ 0)
D Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

mgallien · 2025-10-06T09:05:36Z

src/libsync/logger.cpp

 #include <QStringList>
-#include <QtGlobal>
 #include <QTextCodec>
+#include <QtGlobal>


is this change really needed ?

mgallien · 2025-10-06T09:06:38Z

src/libsync/logger.cpp

+namespace OCC
+{


please revert
current style is
namespace OCC {

mgallien · 2025-10-06T09:21:11Z

src/libsync/logger.cpp

 {
-    if (_logstream)
-    {
+    if (_logstream) {


please split code style fixes into a separate commit
makes it easier to selectively revert or test changes while keeping code style improvements

mgallien · 2025-10-06T09:21:21Z

src/libsync/logger.cpp

    QFile file(dir);
    file.setPermissions(perm);
-    
+


mgallien · 2025-10-06T09:21:47Z

src/libsync/logger.cpp

        int maxNumber = -1;
        const auto collidingFileNames = dir.entryList({QStringLiteral("%1.*").arg(newLogName)}, QDir::Files, QDir::Name);
-        for(const auto &fileName : collidingFileNames) {
+        for (const auto &fileName : collidingFileNames) {


mgallien · 2025-10-06T09:21:55Z

src/libsync/logger.cpp

        auto previousLog = QString{};
-        switch (type)
-        {
+        switch (type) {


mgallien · 2025-10-06T09:24:09Z

src/libsync/logger.cpp

 {
-    qSetMessagePattern(QStringLiteral("%{time yyyy-MM-dd hh:mm:ss:zzz} [ %{type} %{category} %{file}:%{line} "
-                                      "]%{if-debug}\t[ %{function} ]%{endif}:\t%{message}"));
+    qSetMessagePattern(QStringLiteral("%{time yyyy-MM-dd hh:mm:ss:zzz} [%{type} %{category}]:\t%{message}; %{function} %{if-debug}%{file}%{endif}:%{line}"));


please do not change the order ?
omitting the file may save some characters but we should keep the order unchanged to not break possibly existing parsers for the logs

mgallien · 2025-10-06T09:24:30Z

src/libsync/logger.cpp

 {
    static long long int linesCounter = 0;
-    const auto &msg = qFormatLogMessage(type, ctx, message);
+    QString msg = qFormatLogMessage(type, ctx, message);


Suggested change

QString msg = qFormatLogMessage(type, ctx, message);

const auto msg = qFormatLogMessage(type, ctx, message);

mgallien · 2025-10-06T09:25:01Z

src/libsync/logger.cpp

+            if (_doFileFlush || _linesCounter >= MaxLogLinesBeforeFlush || type == QtMsgType::QtWarningMsg || type == QtMsgType::QtCriticalMsg
+                || type == QtMsgType::QtFatalMsg) {


same comment (code style)

nilsding

This change won't resolve the leakage of the complete build path though.

The logger macros (and apparently Qt's old SLOT() macro as well?) embed __FILE__ at compile time, so the full path still ends up in the built binaries:

% strings -n 30 ./bin/nextclouddev | egrep '^/.+/desktop/src/gui/' | sort -u | head -n10
/home/jyrki/src/nextcloud/desktop/src/gui/accountmanager.cpp
/home/jyrki/src/nextcloud/desktop/src/gui/accountsettings.cpp
/home/jyrki/src/nextcloud/desktop/src/gui/accountsetupcommandlinemanager.cpp
/home/jyrki/src/nextcloud/desktop/src/gui/accountsetupfromcommandlinejob.cpp
/home/jyrki/src/nextcloud/desktop/src/gui/accountstate.cpp
/home/jyrki/src/nextcloud/desktop/src/gui/addcertificatedialog.cpp
/home/jyrki/src/nextcloud/desktop/src/gui/application.cpp
/home/jyrki/src/nextcloud/desktop/src/gui/application.cpp:633
/home/jyrki/src/nextcloud/desktop/src/gui/callstatechecker.cpp
/home/jyrki/src/nextcloud/desktop/src/gui/caseclashfilenamedialog.cpp

Aiiaiiio · 2025-10-06T12:30:24Z

I feel resistance to to change the logging pattern. Which I can understand, very minor benefit to it. The security part is somewhat important, but this PR wasn't really about that. Plus what @nilsding found suggests that we should go about it very differently. Should we just ditch this PR?

Signed-off-by: Rello <[email protected]>

will enable to dynamically fetch the list of files inside a folder for now, only basic infrastructure plugging into CfApi is there Signed-off-by: Matthieu Gallien <[email protected]>

will prevent continuous loop of trying to populated the root folder on loop Signed-off-by: Matthieu Gallien <[email protected]>

Signed-off-by: Matthieu Gallien <[email protected]>

will make all folders be initially skipped by cerating an entry into a new type of blacklist upon access by the users they will be removed from this list and synced as normal folders that is a kid of automatic selective sync mechanism (but still creating the folders that are skipped) that would allow the user to know them and access them triggering the dynamic fetching mechanism Signed-off-by: Matthieu Gallien <[email protected]>

Signed-off-by: Matthieu Gallien <[email protected]>

should make the logic much more reliable Signed-off-by: Matthieu Gallien <[email protected]>

should enable proper type handling for virtual directories (i.e. on-demand populated folders) Signed-off-by: Matthieu Gallien <[email protected]>

after having populated the entries in a on-demand folder, update its type in database to ensure updated state only virtual folders have the on-demand feature enabled and that should happen only once as we always fully populated a folder Signed-off-by: Matthieu Gallien <[email protected]>

do not forget that a virtual folder is being also a folder Signed-off-by: Matthieu Gallien <[email protected]>

when a folder is being hydrated (is no longer virtual), let's change the DB record type to be an usual folder part of the normal synchronization Signed-off-by: Matthieu Gallien <[email protected]>

Signed-off-by: Matthieu Gallien <[email protected]>

Bumps [cpp-linter/cpp-linter-action](https://github.com/cpp-linter/cpp-linter-action) from 2.16.4 to 2.16.5. - [Release notes](https://github.com/cpp-linter/cpp-linter-action/releases) - [Commits](cpp-linter/cpp-linter-action@7dacd91...b7fbdde) --- updated-dependencies: - dependency-name: cpp-linter/cpp-linter-action dependency-version: 2.16.5 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: Nextcloud bot <[email protected]>

… Account With this change some warnings from QObject like these are gone now: QObject: Cannot create children for a parent that is in a different thread. I suspect this might have been causing some "random" crashes during normal usage as well. QNAMs are supposed to be used from the same thread they were created in, which (as far as I could tell anyway) isn't the case within async image providers... This change is similar to how the `ImageResponse` class inside `src/gui/tray/usermodel.cpp` fetches a remote resource. Signed-off-by: Jyrki Gadinger <[email protected]>

as documented here https://docs.nextcloud.com/server/latest/developer_manual/client_apis/WebDAV/chunking.html#uploading-chunks Signed-off-by: Matthieu Gallien <[email protected]>

Signed-off-by: Rello <[email protected]>

Signed-off-by: Tamás Bari <[email protected]>

…ialogs Changed HTML anchor attribute delimiters from single quotes to double quotes in conflictdialog.cpp and caseclashfilenamedialog.cpp to properly handle file paths containing single quote characters. Fixes issue where clicking links for files with single quotes in their path would fail due to malformed HTML. Signed-off-by: GitHub Copilot <[email protected]> Co-authored-by: Rello <[email protected]>

Use Utility::escape() to HTML-escape file URLs before embedding them in anchor tags. This ensures both single quotes and double quotes (and other HTML special characters) are properly handled. The toHtmlEscaped() function converts: - ' to ' - " to " - & to & - < to < - > to > This fixes the issue where changing from single to double quote delimiters would break with double quotes instead. Signed-off-by: GitHub Copilot <[email protected]> Co-authored-by: nilsding <[email protected]>

Signed-off-by: Nextcloud bot <[email protected]>

In #8860 I noticed that the "insufficient memory error" message is logged if the last error was anything but `ERROR_INSUFFICIENT_BUFFER` - fix comparison operator - format Windows error codes with a hex code and message Signed-off-by: Jyrki Gadinger <[email protected]>

Signed-off-by: Tamás Bari <[email protected]>

Signed-off-by: Nextcloud bot <[email protected]>

Reported at Transifex Signed-off-by: rakekniven <[email protected]>

Signed-off-by: Jyrki Gadinger <[email protected]>

Signed-off-by: Nextcloud bot <[email protected]>

Signed-off-by: Iva Horn <[email protected]>

… dependencies. Signed-off-by: Iva Horn <[email protected]>

Signed-off-by: Nextcloud bot <[email protected]>

Adjust .gitignore to accept a file with 'build' in the name. Signed-off-by: Camila Ayres <[email protected]>

Signed-off-by: Tamás Bari <[email protected]>

This reverts commit 74f0d90efac5c4c3174bec8444efda8ed2295aa0.

This reverts commit c92f947bda408693f239cd2469262477677e0033.

Signed-off-by: Tamás Bari <[email protected]>

Aiiaiiio · 2025-10-14T13:10:19Z

This change won't resolve the leakage of the complete build path though.

The logger macros (and apparently Qt's old SLOT() macro as well?) embed __FILE__ at compile time, so the full path still ends up in the built binaries:

% strings -n 30 ./bin/nextclouddev | egrep '^/.+/desktop/src/gui/' | sort -u | head -n10
/home/jyrki/src/nextcloud/desktop/src/gui/accountmanager.cpp
/home/jyrki/src/nextcloud/desktop/src/gui/accountsettings.cpp
/home/jyrki/src/nextcloud/desktop/src/gui/accountsetupcommandlinemanager.cpp
/home/jyrki/src/nextcloud/desktop/src/gui/accountsetupfromcommandlinejob.cpp
/home/jyrki/src/nextcloud/desktop/src/gui/accountstate.cpp
/home/jyrki/src/nextcloud/desktop/src/gui/addcertificatedialog.cpp
/home/jyrki/src/nextcloud/desktop/src/gui/application.cpp
/home/jyrki/src/nextcloud/desktop/src/gui/application.cpp:633
/home/jyrki/src/nextcloud/desktop/src/gui/callstatechecker.cpp
/home/jyrki/src/nextcloud/desktop/src/gui/caseclashfilenamedialog.cpp

So, I reverted my previous changes. Instead I introduced the -fmacro-prefix-map compile option. This removes the absolute paths from the release build. Now I only have:

> strings /home/tbari/work/nextcloud/desktop/build/Desktop_Qt_6_9_2-Release/bin/nextcloud  | grep tbari
/home/tbari/Qt/6.9.2/gcc_64/lib:/home/tbari/work/nextcloud/desktop/build/Desktop_Qt_6_9_2-Release/bin:/home/tbari/Qt/6.9.2/gcc_64/lib64:

Which is coming from the RUNPATHs. That will be taken care of by the deploy tools.
This options doesn't exist with MSVC, but afaik the default behaviour is to use relative paths there, so we will see... testing now.

github-actions · 2025-10-14T13:18:43Z

Artifact containing the AppImage: nextcloud-appimage-pr-8823.zip

Digest: sha256:2df7868115e5877c1138c611c63d38d532a7457ef9f26b3f7f004e79696c4541

To test this change/fix you can download the above artifact file, unzip it, and run it.

Please make sure to quit your existing Nextcloud app and backup your data.

github-actions · 2025-10-17T02:08:29Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

fix: Convert full source paths to relative in logs

7bdb0d8

Signed-off-by: Tamás Bari <[email protected]>

Aiiaiiio requested review from camilasan, i2h3, mgallien and nilsding as code owners October 2, 2025 09:43

fix: typo in comment

39daf3b

Signed-off-by: Tamás Bari <[email protected]>

mgallien requested changes Oct 2, 2025

View reviewed changes

Aiiaiiio added 2 commits October 2, 2025 15:06

fix: omit filenames from the log in release mode

256430f

Instead of trying to convert them to relative paths. Signed-off-by: Tamás Bari <[email protected]>

fix: removing unnecessary define from CMakeLists.txt

a3f761f

Signed-off-by: Tamás Bari <[email protected]>

Aiiaiiio requested a review from mgallien October 2, 2025 13:30

mgallien requested changes Oct 6, 2025

View reviewed changes

nilsding requested changes Oct 6, 2025

View reviewed changes

Rello and others added 16 commits October 14, 2025 14:55

fix: show account label in tray hover

4a4a717

Signed-off-by: Rello <[email protected]>

feat(vfs/folders): enable on demand fetch of folder content

33b668f

will enable to dynamically fetch the list of files inside a folder for now, only basic infrastructure plugging into CfApi is there Signed-off-by: Matthieu Gallien <[email protected]>

feat(vfs/folders): enable on demand fetch of folder content

1abd860

will enable to dynamically fetch the list of files inside a folder for now, only basic infrastructure plugging into CfApi is there Signed-off-by: Matthieu Gallien <[email protected]>

fix: once a folder has been populated, disable it

9213c25

will prevent continuous loop of trying to populated the root folder on loop Signed-off-by: Matthieu Gallien <[email protected]>

fix: code style fix

4e439d7

Signed-off-by: Matthieu Gallien <[email protected]>

fix: makes sure we do fetch empty folders again

f53af9a

Signed-off-by: Matthieu Gallien <[email protected]>

fix(vfs/folders-on-demand): do not black list virtual folders

7f1661b

Signed-off-by: Matthieu Gallien <[email protected]>

fix(vfs/folders-on-demand): use proper item type for virtual folders

8a2d2d1

should make the logic much more reliable Signed-off-by: Matthieu Gallien <[email protected]>

fix(vfs/windows): handle virtual directories as virtual directories

11c460a

should enable proper type handling for virtual directories (i.e. on-demand populated folders) Signed-off-by: Matthieu Gallien <[email protected]>

fix(vfs/windows): virtual folders are folders

007f778

do not forget that a virtual folder is being also a folder Signed-off-by: Matthieu Gallien <[email protected]>

fix(vfs/windows): properly update DB record when folder is not virtual

17866f8

when a folder is being hydrated (is no longer virtual), let's change the DB record type to be an usual folder part of the normal synchronization Signed-off-by: Matthieu Gallien <[email protected]>

chore(vfs/windows): more logs for fetch placeholders callback

404a09e

Signed-off-by: Matthieu Gallien <[email protected]>

fix(vfs/windows): fix creation of items inside virtual folders

29239db

Signed-off-by: Matthieu Gallien <[email protected]>

fix(vfs/xattr): fix compilation of vfs xattr experimental plug-in

2e87f45

Signed-off-by: Matthieu Gallien <[email protected]>

dependabot bot and others added 26 commits October 14, 2025 14:55

fix(l10n): Update translations from Transifex

cf44a78

Signed-off-by: Nextcloud bot <[email protected]>

chore(chunk-upload): always sent the total size header

f7a62d1

as documented here https://docs.nextcloud.com/server/latest/developer_manual/client_apis/WebDAV/chunking.html#uploading-chunks Signed-off-by: Matthieu Gallien <[email protected]>

Refactor addAccountButton properties for icon sizing

d4c144a

Signed-off-by: Rello <[email protected]>

fix: Removing non-existent properties

1da2fcf

Signed-off-by: Tamás Bari <[email protected]>

fix: Tweaking account dropdown menu style

f7d06b8

Signed-off-by: Tamás Bari <[email protected]>

fix(l10n): Update translations from Transifex

7a4db91

Signed-off-by: Nextcloud bot <[email protected]>

fix: Adding elide to menuitems

08fe60d

Signed-off-by: Tamás Bari <[email protected]>

fix(l10n): Update translations from Transifex

2d1f52b

Signed-off-by: Nextcloud bot <[email protected]>

fix(l10n): Update translations from Transifex

c6ecc77

Signed-off-by: Nextcloud bot <[email protected]>

fix(l10n): Update translations from Transifex

7b4fe33

Signed-off-by: Nextcloud bot <[email protected]>

fix(i18n): Fixed grammar

acee80a

Reported at Transifex Signed-off-by: rakekniven <[email protected]>

fix(test): adapt updated error message strings

a77107c

Signed-off-by: Jyrki Gadinger <[email protected]>

fix(l10n): Update translations from Transifex

54a8f3a

Signed-off-by: Nextcloud bot <[email protected]>

fix(file-provider): Updated logging calls in extension implementation.

8c05564

Signed-off-by: Iva Horn <[email protected]>

fix(file-provider): Updated NextcloudKit and NextcloudFileProviderKit…

a1d2b8c

… dependencies. Signed-off-by: Iva Horn <[email protected]>

fix(l10n): Update translations from Transifex

0eeff94

Signed-off-by: Nextcloud bot <[email protected]>

fix(NextcloudDev): add missing template file.

6712d31

Adjust .gitignore to accept a file with 'build' in the name. Signed-off-by: Camila Ayres <[email protected]>

fix: revert 3cd559f

dbde1f5

Signed-off-by: Tamás Bari <[email protected]>

fix: Revert "fix: typo in comment"

c15e464

This reverts commit 74f0d90efac5c4c3174bec8444efda8ed2295aa0.

fix: Revert "fix: Convert full source paths to relative in logs"

587b531

This reverts commit c92f947bda408693f239cd2469262477677e0033.

fix: Adding -fmacro-prefix-map compiler option to make paths relative

93c1c84

Signed-off-by: Tamás Bari <[email protected]>

github-actions bot added the feedback-requested label Oct 17, 2025


		get_git_head_revision(GIT_REFSPEC GIT_SHA1)

		add_definitions(-DSOURCE_ROOT="${CMAKE_SOURCE_DIR}")

	QString msg = qFormatLogMessage(type, ctx, message);
	const auto msg = qFormatLogMessage(type, ctx, message);

		if (_doFileFlush \|\| _linesCounter >= MaxLogLinesBeforeFlush \|\| type == QtMsgType::QtWarningMsg \|\| type == QtMsgType::QtCriticalMsg
		\|\| type == QtMsgType::QtFatalMsg) {

		namespace OCC
		{

fix: Convert full source paths to relative in logs #8823

Are you sure you want to change the base?

fix: Convert full source paths to relative in logs #8823

Uh oh!

Conversation

Aiiaiiio commented Oct 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Oct 2, 2025

Quality Gate failed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nilsding left a comment

Choose a reason for hiding this comment

Uh oh!

Aiiaiiio commented Oct 6, 2025

Uh oh!

Aiiaiiio commented Oct 14, 2025

Uh oh!

github-actions bot commented Oct 14, 2025

Uh oh!

github-actions bot commented Oct 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants